[eslint-miner] eslint: add require-fs-io-try-catch rule (statSync / readdirSync / copyFileSync / unlinkSync / renameSync) - #47259
Conversation
…yFileSync/unlinkSync/renameSync Add a new custom ESLint rule that flags fs.statSync, fs.readdirSync, fs.copyFileSync, fs.unlinkSync, and fs.renameSync calls in actions/setup/js when they are not wrapped in try/catch. These methods are the next-highest-risk group of synchronous fs calls after readFileSync/writeFileSync/appendFileSync (already covered by require-fs-sync-try-catch). A scan of actions/setup/js found 33 unguarded call sites across files including artifact_client.cjs, check_workflow_timestamp.cjs, comment_memory_helpers.cjs, merge_remote_agent_github_folder.cjs, and send_otlp_span.cjs. All five methods throw synchronously on ENOENT, EACCES, EBUSY, etc. Without a try/catch, the error propagates as an unhandled exception that crashes the action step with no useful diagnostic message. The rule reuses the createFsSyncMethodResolver / isInsideTryBlock helpers from try-catch-rule-utils so resolver coverage (fs import, destructured bindings, computed member access) is consistent with the existing rule family. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business directories: src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds an ESLint rule requiring try/catch protection around five synchronous filesystem operations in action setup scripts.
Changes:
- Detects unguarded filesystem I/O calls and offers fixes.
- Adds rule tests and plugin registration.
- Enables the rule at warning severity.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-fs-io-try-catch.ts |
Implements detection and suggestions. |
eslint-factory/src/rules/require-fs-io-try-catch.test.ts |
Tests supported methods and bindings. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables rule warnings. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
| const FS_IO_METHODS = new Set(["statSync", "readdirSync", "copyFileSync", "unlinkSync", "renameSync"]); | ||
|
|
||
| export const requireFsIoTryCatchRule = createRule({ | ||
| name: "require-fs-io-try-catch", |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean implementation that follows the established require-execsync-try-catch pattern. The rule correctly targets the 5 throwing fs methods, delegates to shared resolver utilities, and the test coverage is thorough. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 13.7 AIC · ⌖ 5.08 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report
📊 Metrics (9 tests)
✅ Test Coverage SummaryValid cases (no false positives):
Invalid cases (violations caught):
Edge cases:
Verdict
Analysis: Tests comprehensively cover the rule's scope using ESLint's
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. 0% implementation tests (threshold: 30%). All 9 tests verify user-visible behavior through ESLint's RuleTester framework: correct code passes without false positives, violations are caught with exact error metadata, and edge cases (destructured imports) are properly handled.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Code duplication: The implementation is a near-exact copy of
require-fs-sync-try-catch.ts. As a third (or fourth) rule of this shape appears, maintenance cost will compound. A shared factory would be worth the one-time investment now. - Test coverage gaps: Destructured-call tests cover only
statSync/readdirSync; the other three methods are untested in that path. - Test structure asymmetry: Valid cases are grouped into one block while invalid cases are split by method — a minor inconsistency that could hide regressions.
Positive Highlights
- ✅ Good choice to use the existing
createFsSyncMethodResolver/isInsideTryBlockinfrastructure rather than reinventing it. - ✅ The PR description is clear and well-evidenced with 33 concrete call sites.
- ✅ Rule is registered as
warnnoterror, which is appropriate for a linter rollout where existing call sites haven't been fixed yet. - ✅ Test file follows the established per-method pattern from the sibling rule.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 28.4 AIC · ⌖ 4.88 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| // fs methods beyond readFileSync/writeFileSync/appendFileSync that throw on I/O failure | ||
| // and appear frequently unguarded in actions/setup/js. | ||
| const FS_IO_METHODS = new Set(["statSync", "readdirSync", "copyFileSync", "unlinkSync", "renameSync"]); |
There was a problem hiding this comment.
[/codebase-design] This rule is near-identical to require-fs-sync-try-catch.ts — same AST visitor, same fixer logic, same message strings, only the method set differs. Consider extracting a shared factory to avoid two parallel implementations drifting apart.
💡 Suggested refactor
Add a small factory in try-catch-rule-utils.ts:
export function createFsMethodTryCatchRule(
name: string,
methods: Set<string>,
description: string
) {
return createRule({ name, meta: { ... description ... }, create(context) { /* shared visitor */ } });
}Both existing rules then become one-liners, and any future rule follows the same pattern at zero cost.
@copilot please address this.
| ], | ||
| invalid: [], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The valid tests group all five methods in one it block, but the invalid tests use one it per method. This asymmetry means a regression where a method is silently dropped from the valid set would only fail if you happened to test that one. Consider mirroring the structure — one combined invalid block, or one valid block per method — so coverage is symmetric.
@copilot please address this.
| }, | ||
| ], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The destructured-valid test covers statSync and readdirSync but not copyFileSync, unlinkSync, or renameSync. Similarly the destructured-invalid test only covers statSync. A regression that broke destructuring detection for the other three methods would go undetected.
💡 Add missing destructured cases
valid: [
`const { statSync } = require("node:fs"); try { statSync(path); } catch (e) {}`,
`const { copyFileSync } = require("fs"); try { copyFileSync(src, dest); } catch (e) {}`,
`const { unlinkSync } = require("fs"); try { unlinkSync(path); } catch (e) {}`,
`const { renameSync } = require("fs"); try { renameSync(a, b); } catch (e) {}`,
],
invalid: [
{ code: `const { copyFileSync } = require("fs"); copyFileSync(src, dest);`, errors: [{ messageId: "requireTryCatch" }] },
{ code: `const { unlinkSync } = require("fs"); unlinkSync(path);`, errors: [{ messageId: "requireTryCatch" }] },
],@copilot please address this.
|
@copilot merge main and recompile |
…fs-io-try-catch-ba31b9f32b87928e # Conflicts: # eslint-factory/eslint.config.cjs # eslint-factory/src/index.ts Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new ESLint rule
require-fs-io-try-catchto theeslint-factoryplugin. The rule enforces that synchronous filesystem I/O methods —statSync,readdirSync,copyFileSync,unlinkSync, andrenameSync— are wrapped intry/catchblocks inactions/setup/jsscripts.These methods throw synchronously on missing files, permission errors (
EACCES), busy resources (EBUSY), and other I/O failures. An unhandled throw crashes the GitHub Action without surfacing a useful diagnostic message.Changes
eslint-factory/src/rules/require-fs-io-try-catch.tstry-catch-rule-utilshelperseslint-factory/src/rules/require-fs-io-try-catch.test.tseslint-factory/src/index.tsrequireFsIoTryCatchRulein the plugin rules registryeslint-factory/eslint.config.cjswarnseverity in the plugin configeslint-factory/README.mdRule behaviour
Detected forms:
fs.statSync(path)— member expression on a knownrequire("fs")bindingfs["readdirSync"](dir)— computed string-literal property accessconst { unlinkSync } = require("fs")— destructured CJS bindingimport * as fs from "fs"; fs.copyFileSync(src, dest)— ESM namespace importimport { renameSync } from "fs"; renameSync(src, dest)— ESM named importstatSync(path)— bare unbound identifier (not locally declared)Out of scope: non-
fs/node:fssources (e.g.mockFs.statSync);existsSync;readFileSync/writeFileSync/appendFileSync(covered byrequire-fs-sync-try-catch).Suggestion fixer: wraps the enclosing statement in
try { ... } catch (err) { throw new Error("fs.<method> failed: ...", { cause: err }); }with preserved indentation.Breaking changes
None. The rule is added at
warnseverity; no existing valid code is flagged.